Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add pprof to all VPA components #7153

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Add pprof to all VPA components #7153

merged 1 commit into from
Sep 25, 2024

Conversation

adrianmoisey
Copy link
Member

@adrianmoisey adrianmoisey commented Aug 10, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add proof to all 3 VPA components.

Which issue(s) this PR fixes:

Fixes #6946

Special notes for your reviewer:

The change was inspired by the cluster-autoscaler implementation of pprof. I figured it made sense to move a lot of the logic out of the metrics package, as that code handled metrics, health check and pprof.

EDIT: I don't know if I handled the dependencies and vendoring correctly, so I left them in their own commit in case that needs to be changed.

Does this PR introduce a user-facing change?

Add --profiling parameter to all 3 VPA components to enable pprof

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 10, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @adrianmoisey. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 10, 2024
@k8s-ci-robot k8s-ci-robot requested a review from kgolab August 10, 2024 12:40
@adrianmoisey
Copy link
Member Author

@kwiesmueller any chance you can look at this?

@kwiesmueller
Copy link
Member

/assign @voelzmo @raywainman

raywainman
raywainman reviewed Sep 5, 2024
View reviewed changes
vertical-pod-autoscaler/go.mod Outdated
@@ -13,6 +13,7 @@ require (
golang.org/x/time v0.4.0
k8s.io/api v0.28.3
k8s.io/apimachinery v0.28.3
k8s.io/apiserver v0.0.0-00010101000000-000000000000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird that it is using a pseudo-version number... Do you know why it is importing this?

What happens if you try to pull in a specific version? Maybe we can use the same as cluster autoscaler?

autoscaler/cluster-autoscaler/go.mod

Line 47 in 386f0f7

k8s.io/apiserver v0.31.0-beta.0

(I'm not an expert in this so sorry if this is totally off - this just looks weird to me)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 679d6bd fixes it
I used 0.28.3, since that's what the VPA currently uses

vertical-pod-autoscaler/pkg/admission-controller/main.go Outdated
metrics_admission.Register()

go func() {
pathRecorderMux := mux.NewPathRecorderMux("vertical-pod-autoscaler")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reading the documentation and it looks like this name is used for logging, should we make this unique across each binary?

eg. here we would make it vertical-pod-autoscaler-admission-controller

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e4fc614

@raywainman
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2024
@raywainman
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 5, 2024
@adrianmoisey adrianmoisey mentioned this pull request Sep 6, 2024
Closed
11 tasks
@adrianmoisey
Copy link
Member Author

/assign voelzmo

@adrianmoisey
Copy link
Member Author

Hey @voelzmo can you approve this?

kwiesmueller
kwiesmueller requested changes Sep 24, 2024
View reviewed changes
vertical-pod-autoscaler/pkg/admission-controller/main.go Outdated
metrics_admission.Register()

go func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to put this block into some shared util package?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7ec04b3

vertical-pod-autoscaler/pkg/admission-controller/main.go Outdated
pathRecorderMux := mux.NewPathRecorderMux("vertical-pod-autoscaler-admission-controller")

if *enableProfiling {
routes.Profiling{}.Install(pathRecorderMux)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is depending on the apiserver package the best practice here? I'm not sure if that's entirely safe as they may add profiling things that don't fully match.
Might there be something similar focused on api clients (as we're not really apiservers, even though at least the admission controller is serving).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, valid point.
I can't any client side library that includes a pprof function that can be reused.
Since your other comment is suggesting that this chunk of code gets moved to a central place to be re-used, I'll just manually setup the pprof paths there, rather than bringing in a new dependancy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7ec04b3

Something I realised, is that I copied the cluster-autoscaler way of adding pprof, see:

autoscaler/cluster-autoscaler/main.go

Lines 689 to 691 in 77bf580

if *enableProfiling {
routes.Profiling{}.Install(pathRecorderMux)
}

I do agree that for adding pprof it doesn't make sense to add an entire new dependency, so may be that's an opportunity to go clean up cluster-autoscaler too.

@kwiesmueller
Copy link
Member

Thanks for sending this! Very nice to get profiling added.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 25, 2024
kwiesmueller
kwiesmueller approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member

@kwiesmueller kwiesmueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianmoisey, kwiesmueller

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 25, 2024
@k8s-ci-robot k8s-ci-robot merged commit 1c35e11 into kubernetes:master Sep 25, 2024
7 checks passed
@ialidzhikov ialidzhikov mentioned this pull request Jan 21, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raywainman raywainman raywainman left review comments

@kwiesmueller kwiesmueller kwiesmueller approved these changes

@kgolab kgolab Awaiting requested review from kgolab

Assignees

@raywainman raywainman

@voelzmo voelzmo

@kwiesmueller kwiesmueller

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pprof for vpa-admission-controller
5 participants
@adrianmoisey @k8s-ci-robot @kwiesmueller @raywainman @voelzmo